feat(core): Attach TurboModule breakdown to active spans on spanEnd#6478
feat(core): Attach TurboModule breakdown to active spans on spanEnd#6478alwx wants to merge 10 commits into
Conversation
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog.
🤖 This preview updates automatically when you update the PR. |
|
|
@cursor review |
- Nested `name→method` counter map so identifiers with any character can't collide (also removes a stray NUL byte in the compound key). - Fire per-record observers even when `enableAggregateStats: false`, and apply `ignoreTurboModules` whenever any consumer of the record path is active. Fixes span attribution + slow-call breadcrumb going silent when aggregate stats are opted out. - Snapshot the set of open windows at call start via a new `notifyTurboModuleCallStart` hook, so async calls that outlive their originating span still credit that span. Late-settling records re-emit `setAttributes` on the (now closed) span. - CHANGELOG entry references PR #6478 instead of the issue. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds per-span attribution to `turboModuleContextIntegration`: on root
span end, writes `turbo_module.<name>.<method>.{call_count,duration_ms,
error_count}` attributes plus summary keys. Async calls above
`slowCallThresholdMs` (default 500ms) also emit a `native.turbo_module`
breadcrumb. New knobs: `enableSpanAttribution`, `slowCallThresholdMs`,
`maxTopModulesPerSpan`.
Closes #6165.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Nested `name→method` counter map so identifiers with any character can't collide (also removes a stray NUL byte in the compound key). - Fire per-record observers even when `enableAggregateStats: false`, and apply `ignoreTurboModules` whenever any consumer of the record path is active. Fixes span attribution + slow-call breadcrumb going silent when aggregate stats are opted out. - Snapshot the set of open windows at call start via a new `notifyTurboModuleCallStart` hook, so async calls that outlive their originating span still credit that span. Late-settling records re-emit `setAttributes` on the (now closed) span. - CHANGELOG entry references PR #6478 instead of the issue. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
`startObserver` used to skip writing to `pendingCallWindows` when the snapshot was empty. When the call settled, `recordObserver` couldn't find the recordId and fell back to the currently-open windows — crediting spans that opened *after* the call started. Common case: an async TurboModule call kicked off during app init, before any nav / user span has opened. Always record a snapshot (even an empty one), and reserve the currently-open fallback for records without a recordId (direct `recordTurboModuleCall` callers in tests). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Clear per-method attribute keys from a previous emit that no longer fit in the top-N. `setAttributes` merges, so without this a late-settling async call that re-ranks the top-N would leave dropped rows on the span. - Cap `pendingCallWindows` at 1024 so abandoned (never-settling) async promises can't pin `WindowState` indefinitely. Evicted entries are silently dropped on late settle rather than mis-attributed. - Correct the misleading comment about the WeakMap — it enables O(1) `spanEnd` lookup; it does not protect against leaks (the parallel `openWindowList` holds strong refs). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Register the record observer whenever either `enableSpanAttribution` or `slowCallThresholdMs > 0` is active, and gate span-attribution and breadcrumb logic separately inside. Previously the breadcrumb code lived inside the `enableSpanAttribution` block, so disabling span attribution silently turned off breadcrumbs — contradicting the documented independent `slowCallThresholdMs` knob. `setIgnoredTurboModules` also now applies in breadcrumbs-only mode. - Sync calls skip `pendingCallWindows`. Sync start + settle happen in the same turn, so they can credit `openWindowList` directly. Skipping them also prevents sync bursts from evicting genuine async entries under the MAX_PENDING_CALL_WINDOWS cap. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
📲 Install BuildsAndroid
|
antonis
left a comment
There was a problem hiding this comment.
Overall the PR LGTM once the CI is 🟢 (apart from the known issue related failures).
I would advocate towards making the changes opt-in / experimental first to gather some feedback from users (track adoption similar to this) and verify stability before releasing for all. Wdyt?
`wrapTurboModule` always calls `notifyTurboModuleCallStart` with kind `'sync'` and only relabels to `'async'` once the return value is known to be thenable. Gating the start observer on `kind === 'async'` therefore silently dropped every async attribution in production. Revert to snapshotting on every start — sync entries settle in the same synchronous turn and are removed by their paired record, so they don't accumulate under normal traffic. Also add the missing blank line between the Features and Fixes sections in the CHANGELOG. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5c098eb. Configure here.
Android (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| f3215d3+dirty | 411.11 ms | 454.38 ms | 43.27 ms |
| d2eadf8+dirty | 414.64 ms | 454.56 ms | 39.92 ms |
| b0d3373+dirty | 557.66 ms | 579.42 ms | 21.76 ms |
| 27d9693+dirty | 419.08 ms | 469.12 ms | 50.04 ms |
| 4e0b819+dirty | 420.56 ms | 470.08 ms | 49.52 ms |
| 9474ead+dirty | 411.45 ms | 446.80 ms | 35.35 ms |
| 0bd8916+dirty | 412.77 ms | 451.31 ms | 38.54 ms |
| f9c1ed4+dirty | 431.00 ms | 466.22 ms | 35.22 ms |
| 3d536d1+dirty | 524.34 ms | 547.32 ms | 22.98 ms |
| 04207c4+dirty | 459.19 ms | 518.54 ms | 59.35 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| f3215d3+dirty | 48.30 MiB | 53.49 MiB | 5.19 MiB |
| d2eadf8+dirty | 48.30 MiB | 53.48 MiB | 5.18 MiB |
| b0d3373+dirty | 48.30 MiB | 53.58 MiB | 5.28 MiB |
| 27d9693+dirty | 49.74 MiB | 55.09 MiB | 5.34 MiB |
| 4e0b819+dirty | 49.74 MiB | 54.81 MiB | 5.07 MiB |
| 9474ead+dirty | 48.30 MiB | 53.61 MiB | 5.30 MiB |
| 0bd8916+dirty | 48.30 MiB | 53.57 MiB | 5.26 MiB |
| f9c1ed4+dirty | 49.74 MiB | 54.86 MiB | 5.12 MiB |
| 3d536d1+dirty | 49.74 MiB | 55.26 MiB | 5.52 MiB |
| 04207c4+dirty | 43.75 MiB | 48.12 MiB | 4.37 MiB |
iOS (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b9bebee+dirty | 3850.15 ms | 1227.51 ms | -2622.64 ms |
| 3d377b5+dirty | 1218.48 ms | 1219.51 ms | 1.03 ms |
| 1a2e7e0+dirty | 3842.49 ms | 1220.04 ms | -2622.45 ms |
| e5bb5f6+dirty | 3826.14 ms | 1212.24 ms | -2613.90 ms |
| 6acdf1d+dirty | 3844.33 ms | 1212.96 ms | -2631.38 ms |
| 41d6254+dirty | 3845.71 ms | 1224.51 ms | -2621.20 ms |
| 9210ae6+dirty | 3815.93 ms | 1214.14 ms | -2601.79 ms |
| c004dae+dirty | 3850.32 ms | 1227.79 ms | -2622.53 ms |
| 4e0b819+dirty | 3839.05 ms | 1210.75 ms | -2628.30 ms |
| 0b5120f+dirty | 3838.39 ms | 1232.91 ms | -2605.48 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b9bebee+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| 3d377b5+dirty | 3.38 MiB | 4.76 MiB | 1.38 MiB |
| 1a2e7e0+dirty | 4.98 MiB | 6.46 MiB | 1.49 MiB |
| e5bb5f6+dirty | 4.98 MiB | 6.51 MiB | 1.53 MiB |
| 6acdf1d+dirty | 4.98 MiB | 6.51 MiB | 1.53 MiB |
| 41d6254+dirty | 5.15 MiB | 6.70 MiB | 1.54 MiB |
| 9210ae6+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| c004dae+dirty | 5.15 MiB | 6.67 MiB | 1.51 MiB |
| 4e0b819+dirty | 4.98 MiB | 6.46 MiB | 1.49 MiB |
| 0b5120f+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
iOS (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b0d3373+dirty | 3842.49 ms | 1218.49 ms | -2624.00 ms |
| ef27341+dirty | 3835.20 ms | 1212.23 ms | -2622.97 ms |
| e5bb5f6+dirty | 3825.74 ms | 1217.30 ms | -2608.43 ms |
| 44c8b3f+dirty | 3849.24 ms | 1209.94 ms | -2639.31 ms |
| 68672fc+dirty | 3832.22 ms | 1228.29 ms | -2603.93 ms |
| 5748023+dirty | 3844.74 ms | 1225.49 ms | -2619.26 ms |
| 6acdf1d+dirty | 3835.35 ms | 1218.30 ms | -2617.06 ms |
| 2c735cc+dirty | 1223.33 ms | 1224.38 ms | 1.04 ms |
| 100ce80+dirty | 3843.57 ms | 1226.46 ms | -2617.12 ms |
| 9210ae6+dirty | 3834.11 ms | 1216.64 ms | -2617.47 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b0d3373+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| ef27341+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| e5bb5f6+dirty | 4.98 MiB | 6.51 MiB | 1.53 MiB |
| 44c8b3f+dirty | 5.15 MiB | 6.66 MiB | 1.51 MiB |
| 68672fc+dirty | 5.15 MiB | 6.71 MiB | 1.55 MiB |
| 5748023+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| 6acdf1d+dirty | 4.98 MiB | 6.51 MiB | 1.53 MiB |
| 2c735cc+dirty | 3.38 MiB | 4.74 MiB | 1.35 MiB |
| 100ce80+dirty | 5.15 MiB | 6.67 MiB | 1.51 MiB |
| 9210ae6+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
Android (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| a50b33d+dirty | 353.21 ms | 398.48 ms | 45.27 ms |
| d038a14+dirty | 405.08 ms | 444.36 ms | 39.28 ms |
| 68672fc+dirty | 407.55 ms | 442.96 ms | 35.41 ms |
| ad66da3+dirty | 411.49 ms | 449.38 ms | 37.89 ms |
| f170ec3+dirty | 505.96 ms | 551.88 ms | 45.92 ms |
| 27d9693+dirty | 438.63 ms | 514.08 ms | 75.46 ms |
| 41d6254+dirty | 406.20 ms | 445.52 ms | 39.32 ms |
| 68ae91b+dirty | 515.04 ms | 578.08 ms | 63.04 ms |
| 5ee78d6+dirty | 411.18 ms | 437.83 ms | 26.65 ms |
| ae37560+dirty | 428.96 ms | 456.86 ms | 27.90 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| a50b33d+dirty | 43.94 MiB | 48.94 MiB | 5.00 MiB |
| d038a14+dirty | 48.30 MiB | 53.49 MiB | 5.19 MiB |
| 68672fc+dirty | 48.30 MiB | 53.61 MiB | 5.31 MiB |
| ad66da3+dirty | 48.30 MiB | 53.49 MiB | 5.19 MiB |
| f170ec3+dirty | 48.30 MiB | 53.57 MiB | 5.26 MiB |
| 27d9693+dirty | 49.74 MiB | 55.09 MiB | 5.34 MiB |
| 41d6254+dirty | 48.30 MiB | 53.60 MiB | 5.30 MiB |
| 68ae91b+dirty | 49.74 MiB | 54.79 MiB | 5.05 MiB |
| 5ee78d6+dirty | 48.30 MiB | 53.58 MiB | 5.28 MiB |
| ae37560+dirty | 48.30 MiB | 53.60 MiB | 5.29 MiB |
- Late-settling async records after `spanEnd` write via
`span.setAttributes` to a frozen span, which the Sentry SDK silently
drops. Also buffer the attribute payload by span_id and merge into
`event.contexts.trace.data` in `processEvent` so the update lands on
the transaction that ships. `MAX_PENDING_SPAN_ATTRIBUTES` caps the
buffer against sampled-out transactions never coming back to claim
their entry.
- Escape `.` in module/method names when composing attribute keys.
`turbo_module.${name}.${method}.*` collides otherwise —
`(name="a.b", method="c")` and `(name="a", method="b.c")` both produce
`turbo_module.a.b.c.*` and overwrite each other. Same fix applied to
the aggregate flush serialisation.
- Fix the pendingCallWindows cap-eviction test: previously the fillers
never settled, so `attachWindowToSpan` returned early on empty
counters and the assertion loop never ran. Settle a filler so the
eviction outcome is actually verified.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Prune verbose block comments that restated the code or referenced issues. Kept one-line WHYs where the reasoning is load-bearing (frozen-span setAttributes, dot-collision escape, merge semantics). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
| pendingSpanAttributes.delete(oldest); | ||
| } | ||
| } | ||
| pendingSpanAttributes.set(spanId, attributes); |
There was a problem hiding this comment.
Bug: A race condition causes data from late-settling async TurboModule calls to be lost because processEvent runs before the async call completes, deleting the pending attributes prematurely.
Severity: MEDIUM
Suggested Fix
Modify attachWindowToSpan to merge new attributes with any existing attributes for a given spanId in pendingSpanAttributes, rather than overwriting them with set. This ensures that if attachWindowToSpan is called multiple times for the same span (once at spanEnd and again for a late-settling record), all data is accumulated. The processEvent function can then consume the complete set of attributes.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/core/src/js/integrations/turboModuleContext.ts#L403
Potential issue: A race condition can lead to the loss of TurboModule performance data.
If a transaction event is processed before a related asynchronous TurboModule call
completes, the data from that late-settling call is lost. This happens because
`processEvent` reads and deletes the pending attributes for a span from
`pendingSpanAttributes`. When the async call later finishes, it adds its data back to
`pendingSpanAttributes`, but `processEvent` has already run for that transaction, so the
new data is never sent. The fallback of writing to the span via `setAttributes` is
ineffective as it's a no-op on a finished span.

📢 Type of change
📜 Description
On root
spanEnd, attach per-(module, method)TurboModule call counts / duration / errors asturbo_module.<name>.<method>.*attributes on the span, plus summary keys. Async calls overslowCallThresholdMs(default 500ms) emit anative.turbo_modulebreadcrumb.New options on
turboModuleContextIntegration:enableSpanAttribution(default on),slowCallThresholdMs,maxTopModulesPerSpan.Fed by a new per-record observer API on the JS aggregator so open spans can accumulate independently of the transaction / periodic flush drain path.
💡 Motivation and Context
Closes #6165
💚 How did you test it?
Unit tests for the span-attribution + breadcrumb paths and the observer contract.
yarn test,yarn lint,yarn circularDepCheck,yarn api-report:checkpass locally.📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
Docs in #6168.